Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use webgl for cursor #44

Merged
merged 27 commits into from
Nov 15, 2020
Merged

Use webgl for cursor #44

merged 27 commits into from
Nov 15, 2020

Conversation

smolck
Copy link
Owner

@smolck smolck commented Nov 1, 2020

Should fix #42, also removes code from src/core/input.ts for remapping input keys.

@romgrk
Copy link

romgrk commented Nov 2, 2020

Usually it's easier to review when the PR is split in smaller self-consistent parts. It also makes discussion on it more focused.

  • Remove key-mapping transform: LGTM. In my profiles there was about 1-1.5ms delay between the keypress event received and when it reached the actual logic, it can only be good to remove delays there.
  • Render cursor: very good. Using the DOM for rendering is awesome and will surely reduce latency, both by being faster than the DOM and by removing layout trashing.
  • Layout timeout: IMHO it would be good to keep this part for a subsequent PR. Focus on the cursor, that will already be awesome because it's such a sensitive part of the UI. Besides, anything that claims to improve performance should provide proofs of such claims, with actual reproducible numbers. By batching all this together I fear that a regression could be introduced.

I'd also suggest to put in place tests that can be run easily and regularly to check if anything introduces a performance regression.

@smolck smolck changed the title [WIP] Various changes (to hopefully improve performance) [WIP] Use webgl for cursor Nov 2, 2020
@romgrk
Copy link

romgrk commented Nov 2, 2020

Note: this doesn't apply anymore because you've removed the timeout stuff but I'll post anyway:

I've tested a bit more, one thing I notice is that you've added 2 timeouts. This is counter-productive. What is happening is the events trigger a first timer, and when this timer is called it renders some stuff plus triggers a second timer, which then finally renders more stuff. You're adding latency here, not removing it. You need to

  • Gather all redraw events from neovim
  • When the timeout is called, process everything, as fast as possible.

I've also noticed that everytime, the text is redrawn before the cursor is moved. I'm not extacly sure which event you're using to move the cursor, is it the uivonim-position one or the ui-event grid_cursor_goto? In any case, the cursor update should happen at the same time as the text update. You can see this for yourself in the profiles, see example below.

Screenshot from 2020-11-01 22-26-36

@smolck
Copy link
Owner Author

smolck commented Nov 2, 2020

Usually it's easier to review when the PR is split in smaller self-consistent parts. It also makes discussion on it more focused.

[...]

Layout timeout: IMHO it would be good to keep this part for a subsequent PR . . . .

Yeah . . . I got a bit carried away with this PR haha, was originally gonna focus on doing the layout thing, but then started doing cursor stuff . . . my bad.

I've tested a bit more, one thing I notice is that you've added 2 timeouts.

TBH I don't even remember adding that second webgl timeout, or why I did it. Just removing that extra timeout should be enough to make things work though, right? Or will it require more work than that?

I've also noticed that everytime, the text is redrawn before the cursor is moved. I'm not extacly sure which event you're using to move the cursor, is it the uivonim-position one or the ui-event grid_cursor_goto? In any case, the cursor update should happen at the same time as the text update. You can see this for yourself in the profiles, see example below.

Yeah, I've noticed the cursor lagging behind, just have yet to look into exactly why it's happening. The grid_cursor_goto event is the one that moves the cursor, uivonim-position is just a state thing iiuc.

I'd also suggest to put in place tests that can be run easily and regularly to check if anything introduces a performance regression.

Good idea, but first I need to figure out what those should be, what exactly they'll measure, how they'll work, etc., which I've yet to do.

@smolck
Copy link
Owner Author

smolck commented Nov 3, 2020

I've also noticed that everytime, the text is redrawn before the cursor is moved. I'm not extacly sure which event you're using to move the cursor, is it the uivonim-position one or the ui-event grid_cursor_goto? In any case, the cursor update should happen at the same time as the text update. You can see this for yourself in the profiles, see example below.

This isn't happening anymore it seems; I think it was caused by the double timeout, since the second timeout was delaying the webgl redraw, and thus delaying the cursor redraw.

@romgrk
Copy link

romgrk commented Nov 5, 2020

However, I don't really know how to go about drawing the other cursor cursor shapes, specifically the line shape.

Do you really need to draw them with webgl? Does drawing with the 2d context of the rendering canvas has a substantial cost? Maybe I'd try to draw the cursor to a different canvas/context and composite everything together, cursor layer on top of the rest. I'd think it would be less work for the browser than doing the compositing with the DOM.

@smolck
Copy link
Owner Author

smolck commented Nov 5, 2020

I suppose using webgl is not strictly necessary, but I don’t see a real reason not to use it. There’s already code in the codebase that makes using it fairly straightforward, and if I can simply tweak that to also draw the cursor then I see no real reason not to, unless of course the tweaks slow down rendering, but I haven’t noticed any slowdowns so far.

I’m not sure how much cost (if any) there is with using the 2D context of the canvas API vs. WebGL, but doing that would probably add more complexity and require more code.

Regardless, I’ve figured out the line drawing for the most part; for some reason didn’t think to just make the rectangle that’s already being drawn smaller 😅 Still a bit of work to do, for example the other half of the cursor cell’s background is the wrong color atm, but overall it shouldn’t be as hard as I thought.

@smolck smolck changed the title [WIP] Use webgl for cursor Use webgl for cursor Nov 11, 2020
also add some code back that I removed, use `moveCursor` instead
of `windows.webgl.updateCursorPosition` directly in `grid_cursor_goto`,
and maybe some other things
@smolck
Copy link
Owner Author

smolck commented Nov 12, 2020

I've also noticed that everytime, the text is redrawn before the cursor is moved. I'm not extacly sure which event you're using to move the cursor, is it the uivonim-position one or the ui-event grid_cursor_goto? In any case, the cursor update should happen at the same time as the text update. You can see this for yourself in the profiles, see example below.

Interestingly, it appears the cursor lagging behind is inconsistent across setups. On Sway, for example, the lag is fairly obvious when holding j to scroll down, but on GNOME that doesn't seem to happen . . .

@smolck smolck merged commit b681e59 into master Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor character not looking ok
2 participants